tests/cli/test_cli_local_storage.py (2)

54-73: Parameterize this with the existing full-data migration test to avoid duplicate assertions.

This test mirrors the assertions in the prior data migration test with only CLI args changing. Consider combining them into a single parametrized test.

Suggested refactor

-@freeze_time("2025-04-06 13:10:30")
-@pytest.mark.usefixtures("data_migration_file", "schema_migration_file")
-def test_migrate_data_migration(migration_state_file, runner, log):
-    result = runner.invoke(app, ["migrate", "--data"])
+@freeze_time("2025-04-06 13:10:30")
+@pytest.mark.usefixtures("data_migration_file", "schema_migration_file")
+@pytest.mark.parametrize(
+    "cli_args",
+    [
+        ["migrate", "--data"],
+        ["migrate", "--data", "fake_data_file_name"],
+    ],
+)
+def test_migrate_data_migration(cli_args, migration_state_file, runner, log):
+    result = runner.invoke(app, cli_args)
     # asserts unchanged
-
-@freeze_time("2025-04-06 13:10:30")
-@pytest.mark.usefixtures("data_migration_file", "schema_migration_file")
-def test_migrate_data_single_migration(migration_state_file, runner, log):
-    result = runner.invoke(app, ["migrate", "--data", "fake_data_file_name"])
-    # asserts unchanged

As per coding guidelines: "Use @pytest.mark.parametrize when testing permutations of the same behavior" and "Avoid duplicating test logic; extract shared setup into fixtures".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/cli/test_cli_local_storage.py` around lines 54 - 73, Combine the
duplicate assertions by parameterizing the tests: replace
test_migrate_data_single_migration and the existing full-data migration test
with a single parametrized test (e.g., test_migrate_data_variants) that takes
the CLI args (like ["migrate","--data","fake_data_file_name"] and the full-data
args) and the expected migration_id/order_id/started_at/applied_at; use
`@pytest.mark.parametrize` to pass both argument sets and reuse the same fixtures
(migration_state_file, runner, log) and assertions currently inside
test_migrate_data_single_migration (including checking result.exit_code,
migration_state_file JSON contents, and log/output strings) so you avoid
duplicate assertions and keep shared setup in the fixtures.

---

124-141: Parametrize these two error-path tests to reduce duplication.

Both tests run the same command shape and differ only by migration_id and expected message. A single parametrized test keeps the intent while aligning with the test guidelines.

Suggested refactor

-@pytest.mark.usefixtures("data_migration_file")
-def test_migrate_data_single_migration_not_found(runner):
-    result = runner.invoke(app, ["migrate", "--data", "not_existing_migration"])
-
-    assert result.exit_code == 1, result.output
-    assert "Error running data command: Migration not_existing_migration not found" in result.output
-
-
-@pytest.mark.usefixtures("data_migration_file", "schema_migration_file")
-def test_migrate_data_single_migration_wrong_type(runner):
-    result = runner.invoke(app, ["migrate", "--data", "fake_schema_file_name"])
-
-    assert result.exit_code == 1, result.output
-    assert (
-        "Error running data command: Migration fake_schema_file_name is not a data migration"
-        in result.output
-    )
+@pytest.mark.usefixtures("data_migration_file", "schema_migration_file")
+@pytest.mark.parametrize(
+    ("migration_id", "expected_message"),
+    [
+        (
+            "not_existing_migration",
+            "Error running data command: Migration not_existing_migration not found",
+        ),
+        (
+            "fake_schema_file_name",
+            "Error running data command: Migration fake_schema_file_name is not a data migration",
+        ),
+    ],
+)
+def test_migrate_data_single_migration_error_cases(runner, migration_id, expected_message):
+    result = runner.invoke(app, ["migrate", "--data", migration_id])
+
+    assert result.exit_code == 1, result.output
+    assert expected_message in result.output

As per coding guidelines: "Use @pytest.mark.parametrize when testing permutations of the same behavior" and "Avoid duplicating test logic; extract shared setup into fixtures".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/cli/test_cli_local_storage.py` around lines 124 - 141, Replace the two
duplicate tests test_migrate_data_single_migration_not_found and
test_migrate_data_single_migration_wrong_type with a single parametrized test
that accepts migration_id and expected_message, keep the shared
`@pytest.mark.usefixtures`("data_migration_file") plus add "schema_migration_file"
only for the case that needs it (or include both fixtures if simpler), call
runner.invoke(app, ["migrate", "--data", migration_id]) inside the test, and
assert result.exit_code == 1 and that expected_message is in result.output;
reference the existing test names and the runner.invoke(app, ["migrate",
"--data", ...]) call to locate where to consolidate the assertions.

mpt_tool/services/migration_state.py (1)

26-33: Adding a default case is a good defensive coding practice, but not required by current usage.

The save_state method is only called with RUNNING, FAILED, and APPLIED statuses in the codebase. The other enum values (MANUAL_APPLIED and NOT_APPLIED) are read-only states derived from the database and never passed to this method. However, a default case would make the intent explicit and guard against accidental misuse if the method's contract changes.

Suggested defensive addition

         match status:
             case MigrationStatusEnum.APPLIED:
                 state.applied()
             case MigrationStatusEnum.FAILED:
                 state.failed()
             case MigrationStatusEnum.RUNNING:
                 state.start()
+            case _:
+                pass  # Only handle operational state transitions

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mpt_tool/services/migration_state.py` around lines 26 - 33, The match in
save_state currently handles MigrationStatusEnum.APPLIED, FAILED, and RUNNING by
calling state.applied(), state.failed(), and state.start(); add a default branch
to make the intent explicit and guard against misuse—e.g., add a case _ (or
default) that either raises a clear exception (ValueError/TypeError) mentioning
the unexpected MigrationStatusEnum value or logs and no-ops, so unexpected
values like MANUAL_APPLIED or NOT_APPLIED are rejected/handled explicitly in
save_state.

tests/services/test_migration_state.py (1)

52-60: Test function name is misleading.

The test test_save_state_applied is parameterized to test all three status transitions (APPLIED, FAILED, RUNNING), not just APPLIED. Consider renaming to test_save_state or test_save_state_transitions for clarity.

♻️ Proposed fix

-def test_save_state_applied(status, expected_method_call, mocker, mock_state):
+def test_save_state(status, expected_method_call, mocker, mock_state):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/services/test_migration_state.py` around lines 52 - 60, Rename the
misleading test function test_save_state_applied to a more accurate name like
test_save_state_transitions (or test_save_state) since it is parameterized over
multiple statuses (APPLIED, FAILED, RUNNING); update the test function
identifier and any references in the test file or test markers accordingly,
leaving the body unchanged (calls to MigrationStateService.save_state, the
mocked Migration method via mocker.patch.object, and assertions on
mock_method.assert_called_once() and
state_manager.save_state.assert_called_once_with(mock_state) should remain
intact).

mpt_tool/services/migration_state.py (1)

24-34: Match statement doesn't handle all MigrationStatusEnum values.

The match statement only handles APPLIED, FAILED, and RUNNING, but MigrationStatusEnum also includes MANUAL_APPLIED and NOT_APPLIED. If one of those is passed, no transition is applied but save_state still executes, which could lead to silent bugs.

Consider adding a catch-all case that raises an error for unsupported statuses:

♻️ Proposed fix

     def save_state(self, state: Migration, status: MigrationStatusEnum) -> None:
         """Apply status transition and persist migration state."""
         match status:
             case MigrationStatusEnum.APPLIED:
                 state.applied()
             case MigrationStatusEnum.FAILED:
                 state.failed()
             case MigrationStatusEnum.RUNNING:
                 state.start()
+            case _:
+                raise ValueError(f"Unsupported status: {status}")

         self._state_manager.save_state(state)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mpt_tool/services/migration_state.py` around lines 24 - 34, The match in
save_state (in MigrationState.save_state) only handles
MigrationStatusEnum.APPLIED/FAILED/RUNNING; update it to explicitly handle the
remaining enum values (MigrationStatusEnum.MANUAL_APPLIED and
MigrationStatusEnum.NOT_APPLIED) by invoking the appropriate transition methods
on the Migration instance (or, if there are no transitions, document that and do
nothing), and add a catch-all case that raises a clear exception (e.g.,
ValueError) for any unsupported MigrationStatusEnum to avoid silently persisting
an unchanged state; reference the save_state function, MigrationStatusEnum, and
the Migration methods (state.applied, state.failed, state.start, and any
manual/none transition methods) when making the change.

mpt_tool/use_cases/run_single_migration.py (1)

17-25: Use the FileMigrationManager class directly instead of instantiating it.

FileMigrationManager uses @classmethod for all its methods and has no instance state. Instantiating it unnecessarily creates overhead while calling classmethods on the instance. Update the parameter type to type[FileMigrationManager] and pass the class itself:

def __init__(
    self,
    file_migration_manager: type[FileMigrationManager] | None = None,
    state_manager: StateManager | None = None,
    state_service: MigrationStateService | None = None,
):
    self.file_migration_manager = file_migration_manager or FileMigrationManager
    self.state_manager = state_manager or StateManagerFactory.get_instance()
    self.state_service = state_service or MigrationStateService(self.state_manager)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@mpt_tool/use_cases/run_single_migration.py` around lines 17 - 25, The
constructor is instantiating FileMigrationManager even though all its methods
are classmethods; change the parameter and assignment in __init__ to accept and
store the class itself (type[FileMigrationManager]) instead of an instance:
replace the file_migration_manager parameter type and default to
FileMigrationManager (not FileMigrationManager()), and assign
self.file_migration_manager = file_migration_manager or FileMigrationManager;
leave state_manager (StateManagerFactory.get_instance()) and state_service
(MigrationStateService(self.state_manager)) logic unchanged so callers can still
inject those dependencies.

tests/cli/test_cli_local_storage.py (1)

124-139: Parameterize the single-migration error cases to reduce duplication.
These two tests are permutations of the same failure path and can be consolidated.

♻️ Proposed refactor

-@pytest.mark.usefixtures("data_migration_file")
-def test_migrate_data_single_migration_not_found(runner):
-    result = runner.invoke(app, ["migrate", "--data", "not_existing_migration"])
-
-    assert result.exit_code == 1, result.output
-    assert "Error running data command: Migration not_existing_migration not found" in result.output
-
-
-@pytest.mark.usefixtures("data_migration_file", "schema_migration_file")
-def test_migrate_data_single_migration_wrong_type(runner):
-    result = runner.invoke(app, ["migrate", "--data", "fake_schema_file_name"])
-
-    assert result.exit_code == 1, result.output
-    assert (
-        "Error running data command: Migration fake_schema_file_name is not a data migration"
-        in result.output
-    )
+@pytest.mark.usefixtures("data_migration_file", "schema_migration_file")
+@pytest.mark.parametrize(
+    ("args", "expected_message"),
+    [
+        (
+            ["migrate", "--data", "not_existing_migration"],
+            "Error running data command: Migration not_existing_migration not found",
+        ),
+        (
+            ["migrate", "--data", "fake_schema_file_name"],
+            "Error running data command: Migration fake_schema_file_name is not a data migration",
+        ),
+    ],
+)
+def test_migrate_data_single_migration_errors(runner, args, expected_message):
+    result = runner.invoke(app, args)
+
+    assert result.exit_code == 1, result.output
+    assert expected_message in result.output

As per coding guidelines: "Use @pytest.mark.parametrize when testing permutations of the same behavior" and "Avoid duplicating test logic; extract shared setup into fixtures".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/cli/test_cli_local_storage.py` around lines 124 - 139, The two tests
test_migrate_data_single_migration_not_found and
test_migrate_data_single_migration_wrong_type duplicate the same failure path;
replace them with a single parametrized test using pytest.mark.parametrize to
iterate over the migration name and expected error substring, keeping the shared
fixtures (data_migration_file and schema_migration_file) applied as needed;
update the test function (e.g., test_migrate_data_single_migration_errors) to
call runner.invoke(app, ["migrate", "--data", migration_name]), assert exit_code
== 1 and assert the expected error message substring in result.output for each
case so the setup and assertions are not duplicated.